-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: build and use python-build-standalone with official builds #25969
Conversation
782597a
to
3513c49
Compare
4d064e2
to
917ef17
Compare
ecc6cb5
to
19271e0
Compare
18c57c2
to
5d4c9ce
Compare
Fyi, with this PR, local builds that use python-build-standalone can be done like so:
(I added this to README_processing_engine.md; it's of course possible to smooth this out with a helper script) |
2b00534
to
3998d2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, just have a few comments.
@@ -17,7 +17,14 @@ pub enum VenvError { | |||
} | |||
|
|||
fn get_python_version() -> Result<(u8, u8), std::io::Error> { | |||
let output = Command::new("python3") | |||
// linux/osx have python3, but windows only has python. Use python since it is in all of them | |||
let python_exe = if cfg!(target_os = "windows") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about factoring this out into a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that but considering more needs to happen here (#26012, currently assigned to you), I wonder if that can be in a follow-up PR?
626d947
to
eb56f3e
Compare
Not all patchelf versions support --add-rpath for appending to the RPATH, but --set-path can be used with a colon-separated list. Use --set-rpath first for maximum compatibility.
eb56f3e
to
21a6b6c
Compare
Fyi, I incorporated the two doc fixes from @jacksonrnewhouse, dropped the TEMPORARY bits from |
In the interest of time, I'm going to merge this without this code change and then keep an eye on |
This PR enables the processing engine for:
tar.gz
,deb
andrpm
)tar.gz
,brew
(future))zip
)It necessarily removes support for MUSL builds (see
README_processing_engine.md
in this PR). Once we have Linux builds on an olderglibc
(#26010), this should largely not be an issue (again, see theREADME_processing_engine.md
for details).The PR temporarily adjusts
.circleci/config.yaml
to build all of the above artifacts before committing to main so testing can be done by fetching the build artifacts from the 'Artifacts' tab of thebuild-packages
job. These temporary changes are clearly marked with 'TEMPORARY' in the.circleci/config.yaml
file and will be removed before committing.The rust code changes are intentionally of 'POC' quality as there are a few hacks to setup a few things in order to have a good user experience. Since I was sure that these would not remain, I didn't write tests for this code. Thankfully the code changes are minimal (thanks to @jacksonrnewhouse's fine work) and easy to understand, so they can be cleaned up in a subsequent PR. If you'd prefer me to make these changes now, please let me know how you'd like it and I'll take care of it.
I suggest reading
README_processing_engine.md
now to understand why things are the way they are, then come back here and resume your review.Important things:
README_processing_engine.md
for lots of details). Darwin and Windows are not affected and should have great portability.circleci/scripts/package-validation/validate
temporarily usesrpm -ivh --nodeps
until we build on an olderglibc
- Remove rpm validation workaround once have (more) portable GLIBC #26011install_name_tool
andpatchelf
to avoid a wrapper script. Ideally this would be done by the linker, but I couldn't figure out how and I think we're limited in how python-build-standalone is built any way. While it is hacky, it works and is what the tools are for, so there is no pressing need to fix thisinfluxdb3
binary, which isn't the prettiest (seeREADME_processing_engine.md
). It works though, so no pressing need to fixinfluxdb3/src/main.rs
has some hacks to help find things. This is clearly not the right place for this code and there may be ways to not need it at all (Clean up runtime code for finding/using standalone python and entering a venv #26012). Specifically:set_pythonhome()
setsPYTHONHOME
based on where it finds the runtime. I kinda feel like this shouldn't be needed, but the concept of it isn't terrible (even if the location in the code and probably the code itself is)set_pythonpath()
setsPYTHONPATH
and is required on Windows for some reason. This should be fixed properlyinfluxdb3_processing_engine/src/virtualenv.rs
has a few changes (but not enough):get_python_version()
is adjusted to usepython
on Windows aspython3
doesn't exist thereinitialize_venv()
is adjusted to useScripts/activate
and callcmd.exe
on Windows--virtual-env-location
doesn't work on any platform. Linux and OSX can be made to work with venv usingsource /path/to/venv/bin/activate
and launchinginfluxdb3 serve ...
under it (client doesn't need this). Windows needs to setPYTHONPATH=\path\to\venv\Lib\site-packages
. For this and thevirtualenv.rs
stuff, I filed Clean up runtime code for finding/using standalone python and entering a venv #26012 and assigned to @jacksonrnewhouse.circleci/config.yaml
file is wasteful as we're always fetching the same python-build-runtimes. It would be better to save them off in circleci and only refetch if things change. Cache fetched python-build-standalone all.tar.gz tarball in circleci to avoid always fetching #26013README_processing_engine.md
) and can be done in a follow-up PRpip
was tested, notuv
becausepython-build-standalone
haspip
built in (use/path/to/python -m venv <name>
to create the venv, then activate in the normal way and can usepip
(note, Windows needspython -m pip ...
instead; seeREADME_processing_engine.md
), butuv
can be installed bypip
if desired. See Consider integrating uv with standalone python builds #26016Finally, while we may want to change the name of the
system-py
feature, I feel it still has a place for local builds, distribution packages, custom container builds, etc and should be optional by default. SeeREADME_processing_engine.md
for details.Testing performed:
set PYTHONPATH=\path\to\venv\Lib\site-packages
to use the venv (expected, see above)